chore: support conditional triggering of workflow job #799
Conversation
📝 WalkthroughWalkthroughConditional job execution is added to workflows through a new CEL expression field ("if") in JobTemplate. This field is defined in schema specifications, propagated through code generation, and evaluated by the workflow manager at runtime to skip jobs when conditions are false. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager.go`:
- Around line 16-18: The global initialization currently ignores errors from
celutil.NewEnvBuilder().WithMapVariable("inputs").BuildCached(...) so
workflowCelEnv can be nil and later cause a panic when calling Compile; change
the initialization to capture the returned (env, err), check err (and that env
!= nil) and fail fast or return an error from the surrounding init/constructor
(or log and os.Exit) so callers don't hit Compile on a nil env; update any code
that assumed the package-global (e.g., where Compile is invoked) to handle the
initialization error if you convert initialization to a function that returns
(env, error) instead of a silent global.
🧹 Nitpick comments (1)
apps/workspace-engine/pkg/workspace/workflowmanager/manager_test.go (1)
264-265: Consider asserting the identities of the created jobs, not just the count.If a bug caused the wrong job to be skipped (e.g.,
true-jobskipped instead offalse-job), this test would still pass since it only checksLen(wfJobs, 2). Verifying theRefor config of each created workflow job would catch that class of bug.♻️ Suggested addition after the length assertion
wfJobs := store.WorkflowJobs.GetByWorkflowId(wf.Id) assert.Len(t, wfJobs, 2, "should have 2 jobs: always-job and true-job, but not false-job") + + jobRefs := make([]string, len(wfJobs)) + for i, wj := range wfJobs { + jobRefs[i] = wj.Ref + } + assert.Contains(t, jobRefs, "test-job-agent", "always-job should be present") + assert.Contains(t, jobRefs, "test-job-agent", "true-job should be present")Note: Since all three jobs share the same
Ref, you'd need to check by a distinguishing field (e.g., the config or the workflow job's association back to the template job ID) for a truly robust assertion.
| var workflowCelEnv, _ = celutil.NewEnvBuilder(). | ||
| WithMapVariable("inputs"). | ||
| BuildCached(24 * time.Hour) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n apps/workspace-engine/pkg/workspace/workflowmanager/manager.go | head -100Repository: ctrlplanedev/ctrlplane
Length of output: 3801
🏁 Script executed:
find apps/workspace-engine -name "celutil*" -type fRepository: ctrlplanedev/ctrlplane
Length of output: 48
🏁 Script executed:
rg -l "celutil" apps/workspace-engine --type go | head -20Repository: ctrlplanedev/ctrlplane
Length of output: 859
🏁 Script executed:
cat -n apps/workspace-engine/pkg/celutil/cel.goRepository: ctrlplanedev/ctrlplane
Length of output: 8195
🏁 Script executed:
rg "workflowCelEnv" apps/workspace-engine/pkg/workspace/workflowmanager/manager.goRepository: ctrlplanedev/ctrlplane
Length of output: 167
Handle CEL env initialization errors instead of ignoring them.
If BuildCached fails, workflowCelEnv is nil and calling Compile at line 63 will panic. Capture the error and surface it (or fail fast) before evaluation.
🔧 Suggested fix
-var workflowCelEnv, _ = celutil.NewEnvBuilder().
+var workflowCelEnv, workflowCelEnvErr = celutil.NewEnvBuilder().
WithMapVariable("inputs").
BuildCached(24 * time.Hour) func (m *Manager) evaluateJobTemplateIf(jobTemplate oapi.WorkflowJobTemplate, inputs map[string]any) (bool, error) {
+ if workflowCelEnvErr != nil {
+ return false, fmt.Errorf("failed to initialize workflow CEL environment: %w", workflowCelEnvErr)
+ }
prg, err := workflowCelEnv.Compile(*jobTemplate.If)
if err != nil {
return false, fmt.Errorf("failed to compile CEL expression for job %q: %w", jobTemplate.Name, err)
}🤖 Prompt for AI Agents
In `@apps/workspace-engine/pkg/workspace/workflowmanager/manager.go` around lines
16 - 18, The global initialization currently ignores errors from
celutil.NewEnvBuilder().WithMapVariable("inputs").BuildCached(...) so
workflowCelEnv can be nil and later cause a panic when calling Compile; change
the initialization to capture the returned (env, err), check err (and that env
!= nil) and fail fast or return an error from the surrounding init/constructor
(or log and os.Exit) so callers don't hit Compile on a nil env; update any code
that assumed the package-global (e.g., where Compile is invoked) to handle the
initialization error if you convert initialization to a function that returns
(env, error) instead of a silent global.
2 participants
Summary by CodeRabbit
New Features
Tests